New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stream schema #8201
Stream schema #8201
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you @fredericbarthelet, that's a great addition!
Unfortunately CI fails suggesting that proposed schema has some issues (?)
64053a1
to
dc73240
Compare
@medikoo , sorry, made a typo, What kind of tests could I put in place to replace removed test regarding configuration format ? Is there any integration test I could add to cover JSON schema validation ? |
Codecov Report
@@ Coverage Diff @@
## master #8201 +/- ##
==========================================
- Coverage 88.16% 88.11% -0.06%
==========================================
Files 248 248
Lines 9430 9388 -42
==========================================
- Hits 8314 8272 -42
Misses 1116 1116
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fredericbarthelet looks very good, I have just few minor remarks. Let me know if my suggestions are correct
type: { enum: ['sns', 'sqs'] }, | ||
}, | ||
additionalProperties: false, | ||
required: ['arn'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also require 'type'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type is only required when arn is not provided as a string, otherwise it can be computed from the string, thus making it conditionally required. However, I did not managed to achieve such conditional dependency in JSON schema. The properties dependency feature does not allow the same property to have multiple definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I got confused as we support ARN string directly at onFailure
and at onFailure.arn
(and in that case both are equivalent).
In that case maybe it'll be good to configure three variants:
onFailure
as arn stringonFailure
as object witharn
string (notype
)onFailure
as object witharn
andtype
required
oneOf
will make case where arn
string and type
is used, invalid, but I think it's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used anyOf
to prevent such issue.
I updated with 3 possible cases. The same schema was also implemented for stream.arn
and stream.type
property at root. In order to avoid too much duplication, I added a addDefinitionsToSchema
function on configSchemaHandler
and defined mutual schema specific to stream event in this function. @medikoo , WDYT ?
dc73240
to
0eaa004
Compare
additionalProperties: false, | ||
required: ['arn', 'type'], | ||
}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going that route is problematic for error reporting.
As e.g. if one makes error in batchSize
, we will get error for both alternatives, and error normalizer, not being able to decide which alternative was chosen to be followed by user, will expose the error as generic functions[].events[].sns: unsupported configuration format
which doesn't give a hint of what the real error is.
I think this can be cleanly solved solved by using additionalProperties
as a container to define two options for arn
an type
, and keep others defined as it was in previous version
It also doesn't feel good, that we repeat definitions for most of properties, seems error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @medikoo , not sur what you mean by using additionalProperties
. Could you provide an exemple ?
In the meantime, I found a solution detailed in json-schema/json-schema#158 showing how to implement validation based on property value (the correct way of JSON schema is schema dependency based on property existence with dependencies
keyword).
The resulting error message when specifying a CF intrinsic function without type
is Configuration error at 'functions.toto.events[0].stream.destinations.onFailure.arn': should be string
which seems correct. WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @medikoo , not sur what you mean by using additionalProperties. Could you provide an exemple ?
Sorry, that was an invalid suggestion, via additionalProperties
you can enforce schema for other properties, but obviously not one by one, but for all generally.
which seems correct. WDYT ?
Looks great to me!
dc92b20
to
6f27935
Compare
oneOf: [ | ||
{ | ||
properties: { | ||
arn: { $ref: '#/definitions/awsCfFunction' }, | ||
}, | ||
required: ['type'], | ||
}, | ||
{ | ||
properties: { | ||
arn: { $ref: '#/definitions/awsArnString' }, | ||
}, | ||
}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder wouldn't it be more clear (less confusing), to totally discard properties
above, and simply configure both variants fully in scope oneOf
.
In current form it's a bit difficult to read. We have three properties
references that outline two acceptable forms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@medikoo one small problem to that :
- if I remove completly arn definition from
properties
base field, I get a validation error'functions.hello.events[0].stream': unrecognized property 'arn'
sinceadditionalProperties: false
is set. - if I want this error to not be thrown,
arn
needs to be defined outside of theoneOf
clause. All statements inoneOf
are complementary validation logic : both baseobject
JSON schema and one statement ofoneOf
keyword must validate.
I have then 3 solutions :
- keeping the PR as it is now where
oneOf
clauses aboutarn
definitions are subparts of baseproperties.arn
definition. - specifying
arn: {}
definition in the baseproperties
keyword in order to tell that there is anarn
property that will be defined later. The risk is someone opening up to widely theoneOf
clauses about arn and have invalid arn specified. (what I updated the PR to with the last commit, for me this would be the preferred way since there are very small chances above scenario actually happen) - removing
additionalProperties: false
clause
WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also think arn: {}
would be the way to go. So in properties
we indicate purely that it's a recognized property, but full schema is outlined in oneOf
.
It gives clear separation, and minimal risk for mismatch error (e.g. there could be configuration issue, where in properties
we configure schema that doesn't align with one configured in oneOf
options)
6f27935
to
3b17faf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fredericbarthelet Looks great!
Closes: #8034